Skip to content

Add flag to disable ArbOwner outside on-chain execution#4591

Open
bragaigor wants to merge 7 commits intomasterfrom
braga/disable-arbowner-flag
Open

Add flag to disable ArbOwner outside on-chain execution#4591
bragaigor wants to merge 7 commits intomasterfrom
braga/disable-arbowner-flag

Conversation

@bragaigor
Copy link
Copy Markdown
Contributor

@bragaigor bragaigor commented Apr 1, 2026

  • Add --execution.disable-offchain-arbowner node config flag (default false)
  • When enabled, OwnerPrecompile.Call panics if the run context is not IsExecutedOnChain() (blocks ethcall and gas estimation modes)
  • The flag is an atomic.Bool field on the OwnerPrecompile struct, configured during ExecutionNode.Initialize() via gethhook.GetOwnerPrecompile()

The *OwnerPrecompile reference is held in an unexported var in the gethhook package, exposed via GetOwnerPrecompile(). This is necessary because precompile instances are created during gethhook.init() (a void function that runs at package load time), while node config only becomes available later in ExecutionNode.Initialize(). Since init() cannot return values, the pointer must be held somewhere to bridge that gap. An unexported var with a getter is the smallest surface area — set once, immutable after init, and the actual config (disableOffchain) lives on the OwnerPrecompile struct where it belongs.

closes NIT-4744

- When `DisableOffchainArbOwner` is set to `true` in chain config,
  `OwnerPrecompile.Call` panics
- Defaults to `false` — no behavior change unless explicitly opted in

Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 1, 2026

Codecov Report

❌ Patch coverage is 20.00000% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 34.51%. Comparing base (21e0cea) to head (0870413).
⚠️ Report is 25 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4591      +/-   ##
==========================================
+ Coverage   34.30%   34.51%   +0.21%     
==========================================
  Files         498      498              
  Lines       59096    59115      +19     
==========================================
+ Hits        20270    20401     +131     
+ Misses      35238    35082     -156     
- Partials     3588     3632      +44     

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 1, 2026

❌ 12 Tests Failed:

Tests completed Failed Passed Skipped
4693 12 4681 0
View the top 3 failed tests by shortest run time
TestPruningDBSizeReduction
Stack Traces | 0.000s run time
=== RUN   TestPruningDBSizeReduction
--- FAIL: TestPruningDBSizeReduction (0.00s)
TestAliasingFlaky
Stack Traces | -0.000s run time
=== RUN   TestAliasingFlaky
=== PAUSE TestAliasingFlaky
=== CONT  TestAliasingFlaky
    common_test.go:777: BuildL1 deployConfig: DeployBold=true, DeployReferenceDAContracts=false
INFO [04-02|12:52:10.961] Started log indexer
WARN [04-02|12:52:10.961] Getting file info                        dir= error="stat : no such file or directory"
TestBatchPosterL1SurplusMatchesBatchGasFlaky
Stack Traces | 0.540s run time
... [CONTENT TRUNCATED: Keeping last 20 lines]
panic: runtime error: invalid memory address or nil pointer dereference [recovered, repanicked]
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x207c012]

goroutine 73 [running]:
testing.tRunner.func1.2({0x37e1980, 0x61f89b0})
	/opt/hostedtoolcache/go/1.25.8/x64/src/testing/testing.go:1872 +0x237
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.25.8/x64/src/testing/testing.go:1875 +0x35b
panic({0x37e1980?, 0x61f89b0?})
	/opt/hostedtoolcache/go/1.25.8/x64/src/runtime/panic.go:783 +0x132
github.com/offchainlabs/nitro/arbnode.(*InboxTracker).GetBatchCount(0x1a4e7900?)
	/home/runner/work/nitro/nitro/arbnode/inbox_tracker.go:210 +0x12
github.com/offchainlabs/nitro/arbnode.(*InboxTracker).FindInboxBatchContainingMessage(0x0, 0x7)
	/home/runner/work/nitro/nitro/arbnode/inbox_tracker.go:225 +0x2f
github.com/offchainlabs/nitro/system_tests.TestBatchPosterL1SurplusMatchesBatchGasFlaky(0xc0004856c0)
	/home/runner/work/nitro/nitro/system_tests/batch_poster_test.go:839 +0x725
testing.tRunner(0xc0004856c0, 0x41b3880)
	/opt/hostedtoolcache/go/1.25.8/x64/src/testing/testing.go:1934 +0xea
created by testing.(*T).Run in goroutine 1
	/opt/hostedtoolcache/go/1.25.8/x64/src/testing/testing.go:1997 +0x465

📣 Thoughts on this report? Let Codecov know! | Powered by Codecov

Add `--execution.disable-offchain-arbowner` flag to disable ArbOwner
precompile calls outside on-chain execution

Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Comment on lines +20 to +25
var arbOwnerPrecompile *precompiles.OwnerPrecompile

func GetOwnerPrecompile() *precompiles.OwnerPrecompile {
return arbOwnerPrecompile
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The *OwnerPrecompile reference in gethhook is an unexported package-level variable, which we'd normally avoid. Here's why it's necessary:

Precompile instances are created during gethhook.init() — a void function that runs automatically at package load time. Node config (DisableOffchainArbOwner) only becomes available much later in ExecutionNode.Initialize(). Since init() can't return values and has no caller to hand the instance to, the pointer must be held somewhere between creation and configuration. The alternatives we evaluated:

  • storing it in chain config (wrong: this is a node-level concern, not consensus state)
  • a freestanding global atomic bool (worse: config detached from the object it belongs to)
  • or looking it up from the VM precompile maps (fragile: requires unwrapping across multiple layers and picking an arbitrary ArbOS version map)

all the above were all strictly worse. The unexported var with an exported getter is the smallest surface: set exactly once in init(), immutable after that, and the actual config flag (disableOffchain) lives on the OwnerPrecompile struct where it belongs.

In more detail:

  1. gethhook/geth-hook.go init() — runs at package load time (triggered in gethexec/blockchain.go importing gethhook). This is where precompiles.Precompiles() is called, which creates the *OwnerPrecompile instance. That instance gets wrapped in ArbosPrecompileWrapper and stored in the VM's global precompile maps (vm.PrecompiledContractsBeforeArbOS30, etc.). After init() returns, nobody holds a direct reference to the *OwnerPrecompile — it's buried inside the wrapper inside the VM maps.
  2. gethexec.CreateExecutionNode() — runs much later, when the node is actually being set up. This is where configFetcher (which has the DisableOffchainArbOwner flag) first becomes available.
  3. ExecutionNode.Initialize() — called after CreateExecutionNode. This is where we want to call ownerPC.SetDisableOffchain(config.DisableOffchainArbOwner). But we need the *OwnerPrecompile pointer to do that.

The gap: the instance is created in step 1, the config arrives in step 3, and there's no object that naturally carries the pointer from 1 to 3. Hence the global

Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
@bragaigor bragaigor marked this pull request as ready for review April 1, 2026 14:47
@bragaigor bragaigor requested a review from MishkaRogachev April 1, 2026 14:49
ExposeMultiGas bool `koanf:"expose-multi-gas"`
RPCServer rpcserver.Config `koanf:"rpc-server"`
ConsensusRPCClient rpcclient.ClientConfig `koanf:"consensus-rpc-client" reload:"hot"`
DisableOffchainArbOwner bool `koanf:"disable-offchain-arbowner"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you check MessageRunContext, you are going to see that NonMutating happens to be the negation of ExecutedOnChain.
So using non mutating here can be more appropriate than using offchain.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah and furthermore IsNonMutating() and !IsExecutedOnChain() are equivalent:

func (c *MessageRunContext) IsNonMutating() bool {
	return c.runMode == messageGasEstimationMode || c.runMode == messageEthcallMode
}
...
func (c *MessageRunContext) IsExecutedOnChain() bool {
	return c.IsCommitMode() || c.runMode == messageReplayMode || c.runMode == messageRecordingMode
}

@joshuacolvin0 please advice if you had any other plans for DisableOffchainArbOwner as using IsNonMutating seems to be doing exactly what DisableOffchainArbOwner. If we went with IsNonMutating then the following check:

 if wrapper.disableOffchain {                                                                                           
      txProcessor, ok := evm.ProcessingHook.(*arbos.TxProcessor)                                                         
      if !ok || !txProcessor.RunContext().IsExecutedOnChain() {
          return nil, gasSupplied, multigas.ZeroGas(), errors.New("ArbOwner precompile is disabled outside on-chain      
  execution")                                                                                                            
      }                                                                                                                  
  }   

would turn into something like:

  txProcessor, ok := evm.ProcessingHook.(*arbos.TxProcessor)                                                             
  if !ok || txProcessor.MsgIsNonMutating() {                                                                           
      return nil, gasSupplied, multigas.ZeroGas(), errors.New("ArbOwner precompile is disabled outside on-chain 
  execution")                                                                                                            
  }

) ([]byte, uint64, multigas.MultiGas, error) {
if wrapper.disableOffchain.Load() {
txProcessor, ok := evm.ProcessingHook.(*arbos.TxProcessor)
if !ok || !txProcessor.RunContext().IsExecutedOnChain() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task mentions to use IsExecutedOnChain.
Check with who created the task if you can use evm.ProcessinHook.MsgIsNonMutating() instead, which today is the negation of IsExecutedOnChain.
This way you avoid casting.

If not possible then split this if in two.
One to return an error due to casting, and another to return the error that is being proposed here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

related to #4591 (comment)

if wrapper.disableOffchain.Load() {
txProcessor, ok := evm.ProcessingHook.(*arbos.TxProcessor)
if !ok || !txProcessor.RunContext().IsExecutedOnChain() {
return nil, 0, multigas.ComputationGas(gasSupplied), errors.New("ArbOwner precompile is disabled outside on-chain execution")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why returning 0 and multigas.ComputationGas(gasSupplied)?
I am not sure what is the expected behavior, related to those gas values, for eth_estimateGas if it returns an error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best to follow what the rest of Call does so modifying to return:

return nil, gasSupplied, multigas.ZeroGas(), errors.New("ArbOwner precompile is disabled outside on-chain execution")

}
}

func TestDisableOffchainArbOwner(t *testing.T) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't go over this test func yet, but a test in system_tests, sending eth_call and eth_estimateGas, and using the new Execution node config added by this PR, couldn't be used instead of this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved in dce1326

Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
@bragaigor bragaigor assigned joshuacolvin0 and unassigned bragaigor Apr 2, 2026
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants